fix: Make dumb-init PID 1 #4846

lorenz · 2022-02-10T18:58:15Z

dumb-init needs to run as PID 1, otherwise it doesn't do anything.
exec dumb-init so it takes over PID 1 once the entrypoint script is done.

dumb-init needs to run as PID 1, otherwise it doesn't do anything. exec dumb-init so it takes over PID 1 once the entrypoint script is done.

jsjoeio · 2022-02-10T22:52:53Z

@code-asher do you mind reviewing this?

codecov · 2022-02-10T23:01:47Z

Codecov Report

Merging #4846 (bc6c372) into main (8135d2e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4846   +/-   ##
=======================================
  Coverage   69.17%   69.17%           
=======================================
  Files          29       29           
  Lines        1645     1645           
  Branches      363      363           
=======================================
  Hits         1138     1138           
  Misses        430      430           
  Partials       77       77

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03c6224...bc6c372. Read the comment docs.

jawnsy

LGTM, thanks for this!

This change also means that signals to the container are passed to dumb-init, rather than swallowed by the entrypoint script.

Another approach would be to change the entrypoint to invoke dumb-init directly, and use it to run the script, as explained in the docs: https://github.com/Yelp/dumb-init#usage

The entrypoint is defined in the Dockerfile here:

code-server/ci/release-image/Dockerfile

Line 48 in 177f0ed

ENTRYPOINT ["/usr/bin/entrypoint.sh", "--bind-addr", "0.0.0.0:8080", "."]

We could change that line to:

ENTRYPOINT ["/usr/bin/dumb-init", "--"]
CMD ["/usr/bin/entrypoint.sh", "--bind-addr", "0.0.0.0:8080", "."]

and then change the entrypoint script to exec code-server instead of run dumb-init

jsjoeio · 2022-02-11T16:49:47Z

@jawnsy I'll defer you then on this one since this is out of my expertise! If you think this works, feel free to merge.

jawnsy · 2022-02-11T19:17:34Z

I tested this locally using the following steps - an additional benefit of this approach is that dumb-init is able to receive and propagate signals to code-server properly, so it stops more quickly (normally, the first stop signal that Docker sends, SIGTERM, is ignored by the script, and then the following SIGKILL stops the container forcefully).

With the existing approach, we see that the entrypoint script is PID 1 and dumb-init would not be receiving signals directly:

$ docker build . -f ci/release-image/Dockerfile -t code-server
$ docker run -it code-server:latest
[2022-02-11T19:10:59.669Z] info  Wrote default config file to ~/.config/code-server/config.yaml
[2022-02-11T19:11:00.088Z] info  code-server 4.0.2 5cdfe74686aa73e023f8354a9a6014eb30caa7dd
[2022-02-11T19:11:00.089Z] info  Using user-data-dir ~/.local/share/code-server
[2022-02-11T19:11:00.108Z] info  Using config file ~/.config/code-server/config.yaml
[2022-02-11T19:11:00.108Z] info  HTTP server listening on http://0.0.0.0:8080/ 
[2022-02-11T19:11:00.108Z] info    - Authentication is enabled
[2022-02-11T19:11:00.108Z] info      - Using password from ~/.config/code-server/config.yaml
[2022-02-11T19:11:00.108Z] info    - Not serving HTTPS
$ docker ps
CONTAINER ID   IMAGE                COMMAND                  CREATED         STATUS         PORTS      NAMES
647b2233ac6f   code-server:latest   "/usr/bin/entrypoint…"   4 seconds ago   Up 3 seconds   8080/tcp   sleepy_shamir
$ docker exec -it sleepy_shamir ps aux
USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
coder          1  0.0  0.0   2412   516 pts/0    SNs+ 19:10   0:00 /bin/sh /usr/bin/entrypoint.sh --bind-addr 0.0.0.0:8080 
coder         87  0.0  0.0   2164   560 ?        SN   19:10   0:00 dumb-init /usr/bin/code-server --bind-addr 0.0.0.0:8080 
coder         88  0.0  0.0 700428 50568 ?        SNsl 19:10   0:00 /usr/lib/code-server/lib/node /usr/lib/code-server --bin
coder        106  0.0  0.0 634984 49956 ?        SNl  19:10   0:00 /usr/lib/code-server/lib/node /usr/lib/code-server --bin
coder        117  0.0  0.0   9692  3308 pts/1    Rs+  19:11   0:00 ps aux
$ docker stop sleepy_shamir

After we switch, dumb-init is PID 1 and handles signals correctly:

$ docker run -it code-server:latest
[2022-02-11T19:14:00.170Z] info  Wrote default config file to ~/.config/code-server/config.yaml
[2022-02-11T19:14:00.574Z] info  code-server 4.0.2 5cdfe74686aa73e023f8354a9a6014eb30caa7dd
[2022-02-11T19:14:00.575Z] info  Using user-data-dir ~/.local/share/code-server
[2022-02-11T19:14:00.593Z] info  Using config file ~/.config/code-server/config.yaml
[2022-02-11T19:14:00.593Z] info  HTTP server listening on http://0.0.0.0:8080/ 
[2022-02-11T19:14:00.593Z] info    - Authentication is enabled
[2022-02-11T19:14:00.593Z] info      - Using password from ~/.config/code-server/config.yaml
[2022-02-11T19:14:00.593Z] info    - Not serving HTTPS
$ docker ps
CONTAINER ID   IMAGE                COMMAND                  CREATED         STATUS         PORTS      NAMES
73a623fafa4f   code-server:latest   "/usr/bin/entrypoint…"   8 seconds ago   Up 7 seconds   8080/tcp   practical_nash
$ docker exec -it practical_nash ps aux
USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
coder          1  0.0  0.0   2164   560 ?        SNs  19:13   0:00 dumb-init /usr/bin/code-server --bind-addr 0.0.0.0:8080 
coder         87  0.0  0.0 700484 49688 pts/0    SNsl+ 19:13   0:00 /usr/lib/code-server/lib/node /usr/lib/code-server --bi
coder        105  0.0  0.0 635764 52040 pts/0    SNl+ 19:13   0:00 /usr/lib/code-server/lib/node /usr/lib/code-server --bin
coder        116  0.0  0.0   9692  3240 pts/1    Rs+  19:14   0:00 ps aux
$ docker stop practical_nash
practical_nash

I'll merge this after the branch updates and test runs complete. Thanks again for your contribution!

Exec to dumb-init in entrypoint script, so that it can handle signals and reap subprocesses.

Make dumb-init PID 1

e28c5cf

dumb-init needs to run as PID 1, otherwise it doesn't do anything. exec dumb-init so it takes over PID 1 once the entrypoint script is done.

lorenz requested a review from a team February 10, 2022 18:58

jsjoeio changed the title ~~Make dumb-init PID 1~~ fix: Make dumb-init PID 1 Feb 10, 2022

jawnsy approved these changes Feb 11, 2022

View reviewed changes

Merge branch 'main' into patch-2

bc6c372

jawnsy merged commit 102478b into coder:main Feb 11, 2022

lorenz deleted the patch-2 branch February 11, 2022 20:08

TinLe pushed a commit to TinLe/code-server that referenced this pull request Apr 23, 2022

fix: ensure dumb-init is PID 1 (coder#4846)

e88894b

Exec to dumb-init in entrypoint script, so that it can handle signals and reap subprocesses.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Make dumb-init PID 1 #4846

fix: Make dumb-init PID 1 #4846

lorenz commented Feb 10, 2022

jsjoeio commented Feb 10, 2022

codecov bot commented Feb 10, 2022 •

edited

Loading

jawnsy left a comment •

edited

Loading

jsjoeio commented Feb 11, 2022

jawnsy commented Feb 11, 2022

fix: Make dumb-init PID 1 #4846

fix: Make dumb-init PID 1 #4846

Conversation

lorenz commented Feb 10, 2022

jsjoeio commented Feb 10, 2022

codecov bot commented Feb 10, 2022 • edited Loading

Codecov Report

jawnsy left a comment • edited Loading

Choose a reason for hiding this comment

jsjoeio commented Feb 11, 2022

jawnsy commented Feb 11, 2022

codecov bot commented Feb 10, 2022 •

edited

Loading

jawnsy left a comment •

edited

Loading